Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughParses upstream response bodies inside the retry loop, detects and handles body-read Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway
participant Upstream
participant KeyHealth
Client->>Gateway: Send chat request
Gateway->>Upstream: Fetch (attempt N)
alt Upstream returns headers (ok)
Upstream-->>Gateway: Response headers
Gateway->>Gateway: Attempt body read / res.json()
alt body read TimeoutError
Gateway->>KeyHealth: Report upstream body-timeout (include retried flag, routingAttempts)
Gateway->>Gateway: shouldRetryRequest? -> queue retry / increment routingAttempts
Gateway-->>Upstream: Retry fetch (next attempt)
else body read succeeds
Gateway-->>Client: Return parsed JSON (or stream)
end
else Upstream fetch error / non-ok
Gateway->>KeyHealth: Report fetch error (with retried state)
Gateway->>Gateway: shouldRetryRequest? -> retry or return error (may return 504)
end
opt retries exhausted or unrecoverable
Gateway-->>Client: Return 504 Upstream Timeout or appropriate error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 5751-5754: The current call to reportKeyError(envVarName,
configIndex, res.status) can misattribute transport-level failures (e.g.,
body-read timeouts that happen after a 200) to the provider key; change the
logic so that when the failure is a transport-level timeout you call
reportKeyError(envVarName, configIndex, 0) (use status 0 for transport failures)
and otherwise continue passing res.status; locate the call to reportKeyError in
the post-response handling around variables envVarName, configIndex and
res.status and branch on whether the error originated from a body-read/transport
timeout vs. an actual HTTP error.
- Around line 5640-5783: The body-read timeout is using res.status (e.g., 200)
which prevents retries and misclassifies the error; change places that currently
pass res.status for timeouts to use statusCode: 0 instead: call
shouldRetryRequest with statusCode: 0, set errorDetails.statusCode to 0 in the
insertLog payload, set the routingAttempts entry's status_code to 0 and use
getErrorType(0) for error_type, and when calling reportKeyError pass 0 as the
status; update any other uses in this block that rely on res.status for error
classification to use 0 so transport-level timeout is treated as
retryable/transport error.
Comment on lines
+5640
to
+5783
| // At this point, res must be defined and ok (otherwise we would have continued/returned above) | ||
| if (!res || !res.ok) { | ||
| throw new Error("Response not ok after error handling"); | ||
| } | ||
|
|
||
| // Parse response body before exiting retry loop so we can retry on timeout | ||
| // Body read can throw TimeoutError if the abort signal fires during consumption | ||
| try { | ||
| json = await res.json(); | ||
| } catch (bodyError) { | ||
| if (isTimeoutError(bodyError)) { | ||
| const errorMessage = | ||
| bodyError instanceof Error | ||
| ? bodyError.message | ||
| : "Timeout reading response body"; | ||
| logger.warn("Timeout reading response body", { | ||
| usedProvider, | ||
| usedModel, | ||
| initialRequestedModel, | ||
| }); | ||
|
|
||
| // Check if we should retry before logging so we can mark the log as retried | ||
| const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({ | ||
| requestedProvider, | ||
| noFallback, | ||
| statusCode: res.status, | ||
| retryCount: retryAttempt, | ||
| remainingProviders: | ||
| (routingMetadata?.providerScores.length ?? 0) - | ||
| failedProviderIds.size - | ||
| 1, | ||
| usedProvider, | ||
| }); | ||
|
|
||
| const bodyTimeoutPluginIds = plugins?.map((p) => p.id) || []; | ||
| const baseLogEntry = createLogEntry( | ||
| requestId, | ||
| project, | ||
| apiKey, | ||
| providerKey?.id, | ||
| usedModelFormatted, | ||
| usedModelMapping, | ||
| usedProvider, | ||
| initialRequestedModel, | ||
| requestedProvider, | ||
| messages, | ||
| temperature, | ||
| max_tokens, | ||
| top_p, | ||
| frequency_penalty, | ||
| presence_penalty, | ||
| reasoning_effort, | ||
| reasoning_max_tokens, | ||
| effort, | ||
| response_format, | ||
| tools, | ||
| tool_choice, | ||
| source, | ||
| customHeaders, | ||
| debugMode, | ||
| userAgent, | ||
| image_config, | ||
| routingMetadata, | ||
| rawBody, | ||
| null, | ||
| requestBody, | ||
| null, | ||
| bodyTimeoutPluginIds, | ||
| undefined, | ||
| ); | ||
|
|
||
| await insertLog({ | ||
| ...baseLogEntry, | ||
| duration: Date.now() - perAttemptStartTime, | ||
| timeToFirstToken: null, | ||
| timeToFirstReasoningToken: null, | ||
| responseSize: 0, | ||
| content: null, | ||
| reasoningContent: null, | ||
| finishReason: "upstream_error", | ||
| promptTokens: null, | ||
| completionTokens: null, | ||
| totalTokens: null, | ||
| reasoningTokens: null, | ||
| cachedTokens: null, | ||
| hasError: true, | ||
| streamed: false, | ||
| canceled: false, | ||
| errorDetails: { | ||
| statusCode: res.status, | ||
| statusText: "TimeoutError", | ||
| responseText: errorMessage, | ||
| }, | ||
| cachedInputCost: null, | ||
| requestCost: null, | ||
| webSearchCost: null, | ||
| imageInputTokens: null, | ||
| imageOutputTokens: null, | ||
| imageInputCost: null, | ||
| imageOutputCost: null, | ||
| estimatedCost: false, | ||
| discount: null, | ||
| dataStorageCost: "0", | ||
| cached: false, | ||
| toolResults: null, | ||
| retried: willRetrySuccessBodyTimeoutNonStreaming, | ||
| retriedByLogId: willRetrySuccessBodyTimeoutNonStreaming | ||
| ? finalLogId | ||
| : null, | ||
| }); | ||
|
|
||
| // Report key health for environment-based tokens | ||
| if (envVarName !== undefined) { | ||
| reportKeyError(envVarName, configIndex, res.status); | ||
| } | ||
|
|
||
| if (willRetrySuccessBodyTimeoutNonStreaming) { | ||
| routingAttempts.push({ | ||
| provider: usedProvider, | ||
| model: usedModel, | ||
| status_code: res.status, | ||
| error_type: getErrorType(res.status), | ||
| succeeded: false, | ||
| }); | ||
| failedProviderIds.add(usedProvider); | ||
| continue; | ||
| } | ||
|
|
||
| return c.json( | ||
| { | ||
| error: { | ||
| message: `Upstream provider timeout: ${errorMessage}`, | ||
| type: "upstream_timeout", | ||
| param: null, | ||
| code: "timeout", | ||
| }, | ||
| }, | ||
| 504, | ||
| ); | ||
| } | ||
| throw bodyError; | ||
| } | ||
|
|
||
| break; // Fetch and body parse succeeded, exit retry loop |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find shouldRetryRequest function definition
echo "=== Searching for shouldRetryRequest function ==="
rg -n "function shouldRetryRequest|const shouldRetryRequest|export.*shouldRetryRequest" --type ts -A 20 | head -100
echo ""
echo "=== Searching for getErrorType function ==="
rg -n "function getErrorType|const getErrorType|export.*getErrorType" --type ts -A 10 | head -80Repository: theopenco/llmgateway
Length of output: 3192
🏁 Script executed:
#!/bin/bash
# Get context around the shouldRetryRequest calls in the changed lines
echo "=== Context around shouldRetryRequest call at line 5662-5672 (success body timeout) ==="
sed -n '5660,5675p' apps/gateway/src/chat/chat.ts
echo ""
echo "=== Context around shouldRetryRequest call at line 5307 (error body timeout) ==="
sed -n '5305,5320p' apps/gateway/src/chat/chat.tsRepository: theopenco/llmgateway
Length of output: 1227
🏁 Script executed:
#!/bin/bash
# Search for where shouldRetryRequest is defined or imported
echo "=== Import or definition of shouldRetryRequest ==="
rg -n "shouldRetryRequest" apps/gateway/src/chat/chat.ts | head -20Repository: theopenco/llmgateway
Length of output: 588
🏁 Script executed:
#!/bin/bash
# Find isRetryableError implementation
echo "=== Searching for isRetryableError function ==="
rg -n "function isRetryableError|const isRetryableError|export.*isRetryableError" --type ts -A 20Repository: theopenco/llmgateway
Length of output: 1919
🏁 Script executed:
#!/bin/bash
# Also check if isRetryableError is imported from elsewhere
echo "=== Check imports and all references to isRetryableError ==="
rg -n "isRetryableError" apps/gateway/src/chat/tools/retry-with-fallback.tsRepository: theopenco/llmgateway
Length of output: 237
Pass statusCode: 0 for body-read timeouts to enable actual retries
The success-response body timeout block passes statusCode: res.status (200) to shouldRetryRequest, but isRetryableError() only considers 429, 5xx, and 0 as retryable. Status 200 fails the check, preventing retries — defeating the PR's goal.
Similarly, getErrorType(200) returns "upstream_error", misclassifying a timeout as a provider error.
Apply the proposed fix: pass statusCode: 0 for timeout errors (matching the fetch-timeout pattern at line 5029) to signal transport-level failure:
Proposed changes
const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({
requestedProvider,
noFallback,
- statusCode: res.status,
+ statusCode: 0,
retryCount: retryAttempt,
remainingProviders:
(routingMetadata?.providerScores.length ?? 0) -
failedProviderIds.size -
1,
usedProvider,
}); if (willRetrySuccessBodyTimeoutNonStreaming) {
routingAttempts.push({
provider: usedProvider,
model: usedModel,
- status_code: res.status,
- error_type: getErrorType(res.status),
+ status_code: 0,
+ error_type: getErrorType(0),
succeeded: false,
});Also update the error tracking (around line 5729 and 5753):
- reportKeyError(envVarName, configIndex, res.status);
+ reportKeyError(envVarName, configIndex, 0); errorDetails: {
- statusCode: res.status,
+ statusCode: 0,
statusText: "TimeoutError",
responseText: errorMessage,
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // At this point, res must be defined and ok (otherwise we would have continued/returned above) | |
| if (!res || !res.ok) { | |
| throw new Error("Response not ok after error handling"); | |
| } | |
| // Parse response body before exiting retry loop so we can retry on timeout | |
| // Body read can throw TimeoutError if the abort signal fires during consumption | |
| try { | |
| json = await res.json(); | |
| } catch (bodyError) { | |
| if (isTimeoutError(bodyError)) { | |
| const errorMessage = | |
| bodyError instanceof Error | |
| ? bodyError.message | |
| : "Timeout reading response body"; | |
| logger.warn("Timeout reading response body", { | |
| usedProvider, | |
| usedModel, | |
| initialRequestedModel, | |
| }); | |
| // Check if we should retry before logging so we can mark the log as retried | |
| const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({ | |
| requestedProvider, | |
| noFallback, | |
| statusCode: res.status, | |
| retryCount: retryAttempt, | |
| remainingProviders: | |
| (routingMetadata?.providerScores.length ?? 0) - | |
| failedProviderIds.size - | |
| 1, | |
| usedProvider, | |
| }); | |
| const bodyTimeoutPluginIds = plugins?.map((p) => p.id) || []; | |
| const baseLogEntry = createLogEntry( | |
| requestId, | |
| project, | |
| apiKey, | |
| providerKey?.id, | |
| usedModelFormatted, | |
| usedModelMapping, | |
| usedProvider, | |
| initialRequestedModel, | |
| requestedProvider, | |
| messages, | |
| temperature, | |
| max_tokens, | |
| top_p, | |
| frequency_penalty, | |
| presence_penalty, | |
| reasoning_effort, | |
| reasoning_max_tokens, | |
| effort, | |
| response_format, | |
| tools, | |
| tool_choice, | |
| source, | |
| customHeaders, | |
| debugMode, | |
| userAgent, | |
| image_config, | |
| routingMetadata, | |
| rawBody, | |
| null, | |
| requestBody, | |
| null, | |
| bodyTimeoutPluginIds, | |
| undefined, | |
| ); | |
| await insertLog({ | |
| ...baseLogEntry, | |
| duration: Date.now() - perAttemptStartTime, | |
| timeToFirstToken: null, | |
| timeToFirstReasoningToken: null, | |
| responseSize: 0, | |
| content: null, | |
| reasoningContent: null, | |
| finishReason: "upstream_error", | |
| promptTokens: null, | |
| completionTokens: null, | |
| totalTokens: null, | |
| reasoningTokens: null, | |
| cachedTokens: null, | |
| hasError: true, | |
| streamed: false, | |
| canceled: false, | |
| errorDetails: { | |
| statusCode: res.status, | |
| statusText: "TimeoutError", | |
| responseText: errorMessage, | |
| }, | |
| cachedInputCost: null, | |
| requestCost: null, | |
| webSearchCost: null, | |
| imageInputTokens: null, | |
| imageOutputTokens: null, | |
| imageInputCost: null, | |
| imageOutputCost: null, | |
| estimatedCost: false, | |
| discount: null, | |
| dataStorageCost: "0", | |
| cached: false, | |
| toolResults: null, | |
| retried: willRetrySuccessBodyTimeoutNonStreaming, | |
| retriedByLogId: willRetrySuccessBodyTimeoutNonStreaming | |
| ? finalLogId | |
| : null, | |
| }); | |
| // Report key health for environment-based tokens | |
| if (envVarName !== undefined) { | |
| reportKeyError(envVarName, configIndex, res.status); | |
| } | |
| if (willRetrySuccessBodyTimeoutNonStreaming) { | |
| routingAttempts.push({ | |
| provider: usedProvider, | |
| model: usedModel, | |
| status_code: res.status, | |
| error_type: getErrorType(res.status), | |
| succeeded: false, | |
| }); | |
| failedProviderIds.add(usedProvider); | |
| continue; | |
| } | |
| return c.json( | |
| { | |
| error: { | |
| message: `Upstream provider timeout: ${errorMessage}`, | |
| type: "upstream_timeout", | |
| param: null, | |
| code: "timeout", | |
| }, | |
| }, | |
| 504, | |
| ); | |
| } | |
| throw bodyError; | |
| } | |
| break; // Fetch and body parse succeeded, exit retry loop | |
| // At this point, res must be defined and ok (otherwise we would have continued/returned above) | |
| if (!res || !res.ok) { | |
| throw new Error("Response not ok after error handling"); | |
| } | |
| // Parse response body before exiting retry loop so we can retry on timeout | |
| // Body read can throw TimeoutError if the abort signal fires during consumption | |
| try { | |
| json = await res.json(); | |
| } catch (bodyError) { | |
| if (isTimeoutError(bodyError)) { | |
| const errorMessage = | |
| bodyError instanceof Error | |
| ? bodyError.message | |
| : "Timeout reading response body"; | |
| logger.warn("Timeout reading response body", { | |
| usedProvider, | |
| usedModel, | |
| initialRequestedModel, | |
| }); | |
| // Check if we should retry before logging so we can mark the log as retried | |
| const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({ | |
| requestedProvider, | |
| noFallback, | |
| statusCode: 0, | |
| retryCount: retryAttempt, | |
| remainingProviders: | |
| (routingMetadata?.providerScores.length ?? 0) - | |
| failedProviderIds.size - | |
| 1, | |
| usedProvider, | |
| }); | |
| const bodyTimeoutPluginIds = plugins?.map((p) => p.id) || []; | |
| const baseLogEntry = createLogEntry( | |
| requestId, | |
| project, | |
| apiKey, | |
| providerKey?.id, | |
| usedModelFormatted, | |
| usedModelMapping, | |
| usedProvider, | |
| initialRequestedModel, | |
| requestedProvider, | |
| messages, | |
| temperature, | |
| max_tokens, | |
| top_p, | |
| frequency_penalty, | |
| presence_penalty, | |
| reasoning_effort, | |
| reasoning_max_tokens, | |
| effort, | |
| response_format, | |
| tools, | |
| tool_choice, | |
| source, | |
| customHeaders, | |
| debugMode, | |
| userAgent, | |
| image_config, | |
| routingMetadata, | |
| rawBody, | |
| null, | |
| requestBody, | |
| null, | |
| bodyTimeoutPluginIds, | |
| undefined, | |
| ); | |
| await insertLog({ | |
| ...baseLogEntry, | |
| duration: Date.now() - perAttemptStartTime, | |
| timeToFirstToken: null, | |
| timeToFirstReasoningToken: null, | |
| responseSize: 0, | |
| content: null, | |
| reasoningContent: null, | |
| finishReason: "upstream_error", | |
| promptTokens: null, | |
| completionTokens: null, | |
| totalTokens: null, | |
| reasoningTokens: null, | |
| cachedTokens: null, | |
| hasError: true, | |
| streamed: false, | |
| canceled: false, | |
| errorDetails: { | |
| statusCode: 0, | |
| statusText: "TimeoutError", | |
| responseText: errorMessage, | |
| }, | |
| cachedInputCost: null, | |
| requestCost: null, | |
| webSearchCost: null, | |
| imageInputTokens: null, | |
| imageOutputTokens: null, | |
| imageInputCost: null, | |
| imageOutputCost: null, | |
| estimatedCost: false, | |
| discount: null, | |
| dataStorageCost: "0", | |
| cached: false, | |
| toolResults: null, | |
| retried: willRetrySuccessBodyTimeoutNonStreaming, | |
| retriedByLogId: willRetrySuccessBodyTimeoutNonStreaming | |
| ? finalLogId | |
| : null, | |
| }); | |
| // Report key health for environment-based tokens | |
| if (envVarName !== undefined) { | |
| reportKeyError(envVarName, configIndex, 0); | |
| } | |
| if (willRetrySuccessBodyTimeoutNonStreaming) { | |
| routingAttempts.push({ | |
| provider: usedProvider, | |
| model: usedModel, | |
| status_code: 0, | |
| error_type: getErrorType(0), | |
| succeeded: false, | |
| }); | |
| failedProviderIds.add(usedProvider); | |
| continue; | |
| } | |
| return c.json( | |
| { | |
| error: { | |
| message: `Upstream provider timeout: ${errorMessage}`, | |
| type: "upstream_timeout", | |
| param: null, | |
| code: "timeout", | |
| }, | |
| }, | |
| 504, | |
| ); | |
| } | |
| throw bodyError; | |
| } | |
| break; // Fetch and body parse succeeded, exit retry loop |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 5640 - 5783, The body-read
timeout is using res.status (e.g., 200) which prevents retries and misclassifies
the error; change places that currently pass res.status for timeouts to use
statusCode: 0 instead: call shouldRetryRequest with statusCode: 0, set
errorDetails.statusCode to 0 in the insertLog payload, set the routingAttempts
entry's status_code to 0 and use getErrorType(0) for error_type, and when
calling reportKeyError pass 0 as the status; update any other uses in this block
that rely on res.status for error classification to use 0 so transport-level
timeout is treated as retryable/transport error.
Comment on lines
+5751
to
+5754
| // Report key health for environment-based tokens | ||
| if (envVarName !== undefined) { | ||
| reportKeyError(envVarName, configIndex, res.status); | ||
| } |
Contributor
There was a problem hiding this comment.
Reporting a key error with status 200 is misleading.
When the body-read of a successful (200) response times out, the provider key itself isn't at fault — the issue is a transport-level timeout. Reporting this as a key error with status 200 may incorrectly degrade the health score of a healthy key. This is addressed by the suggestion in the prior comment to use status 0 consistently for transport-level failures.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` around lines 5751 - 5754, The current call to
reportKeyError(envVarName, configIndex, res.status) can misattribute
transport-level failures (e.g., body-read timeouts that happen after a 200) to
the provider key; change the logic so that when the failure is a transport-level
timeout you call reportKeyError(envVarName, configIndex, 0) (use status 0 for
transport failures) and otherwise continue passing res.status; locate the call
to reportKeyError in the post-response handling around variables envVarName,
configIndex and res.status and branch on whether the error originated from a
body-read/transport timeout vs. an actual HTTP error.
There was a problem hiding this comment.
Pull request overview
This PR aims to fix timeout retry tracking and enable graceful retries for all timeout scenarios in non-streaming chat completions. The changes move response body parsing inside the retry loop to enable retries on body read timeouts, and add proper retry metadata tracking (retried, retriedByLogId) for both error and success response body timeouts.
Changes:
- Moved
jsonvariable declaration to before the retry loop to allow body parsing inside the loop - Added retry logic for error response body timeouts with proper metadata tracking
- Moved success response body parsing inside the retry loop to enable timeout retries
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+5662
to
+5672
| const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({ | ||
| requestedProvider, | ||
| noFallback, | ||
| statusCode: res.status, | ||
| retryCount: retryAttempt, | ||
| remainingProviders: | ||
| (routingMetadata?.providerScores.length ?? 0) - | ||
| failedProviderIds.size - | ||
| 1, | ||
| usedProvider, | ||
| }); |
There was a problem hiding this comment.
Potential bug: When a timeout occurs while reading the body of a successful response (2xx status code), shouldRetryRequest will always return false because isRetryableError does not consider 2xx status codes as retryable. This means willRetrySuccessBodyTimeoutNonStreaming will always be false, so the retry logic will never execute and the error will be returned immediately with no retry attempt. Consider passing a different status code (like 0 for timeout errors, or 504) to shouldRetryRequest to ensure the timeout is treated as retryable.
Comment on lines
+5751
to
+5754
| // Report key health for environment-based tokens | ||
| if (envVarName !== undefined) { | ||
| reportKeyError(envVarName, configIndex, res.status); | ||
| } |
There was a problem hiding this comment.
Misleading status code in error reporting: When a timeout occurs while reading a successful response body, reportKeyError is called with res.status (which is 2xx). This could incorrectly mark the API key as having an error when the HTTP request actually succeeded - the issue is only with reading the response body. Consider using a timeout-specific status code (e.g., 0 or 504) when reporting this type of error.
Comment on lines
+5760
to
+5761
| status_code: res.status, | ||
| error_type: getErrorType(res.status), |
There was a problem hiding this comment.
Incorrect error_type for body timeout: When logging a routing attempt for a body read timeout on a successful response, getErrorType(res.status) is called with a 2xx status code. This will return "upstream_error", which is semantically incorrect - the upstream request succeeded, but reading the body timed out. Consider using a timeout-specific error type or passing a different status code (e.g., 0 or 504) to getErrorType.
Suggested change
| status_code: res.status, | |
| error_type: getErrorType(res.status), | |
| status_code: 504, | |
| error_type: getErrorType(504), |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/gateway/src/chat/chat.ts`:
- Line 4849: The binding "let json: any" should use a safer type: change it to
"let json: unknown" and then narrow it after calling res.json() before using it
(e.g., with a type guard, instanceof checks, or explicit validation) so
downstream code operates on a known shape; update the code around the res.json()
call (where "json" is assigned) to perform the necessary runtime checks/casts
and only then access properties or cast to a specific interface.
- Around line 5751-5779: After exhausting retries selectNextProvider can return
null and leave `json` undefined, so add a guard after the existing `if (!res ||
!res.ok)` block to verify `json` is set before calling parseProviderResponse; if
`json` is undefined, push a routingAttempts entry for the current `usedProvider`
(matching the pattern used when timeouts occur), mark the provider failed
(failedProviderIds.add(usedProvider)), and return the same upstream timeout 504
JSON response used elsewhere; update the nearby comment (referencing
`parseProviderResponse`, `selectNextProvider`, `res`, and `json`) to note that
`json` may be unset when all providers timeout.
---
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 5662-5672: The retry decision is using res.status (200) so
willRetrySuccessBodyTimeoutNonStreaming is always false; instead detect the
body-read timeout and surface that to shouldRetryRequest (or pass a non-200
sentinel) so the request is considered retryable even when res.ok is true.
Update the call site that sets willRetrySuccessBodyTimeoutNonStreaming (use
variables willRetrySuccessBodyTimeoutNonStreaming, shouldRetryRequest,
res.status, res.ok, retryAttempt, failedProviderIds, requestedProvider,
noFallback, routingMetadata, usedProvider) to pass an extra flag like
bodyReadTimedOut (or replace statusCode with a sentinel when res.ok &&
bodyReadTimedOut) and adjust shouldRetryRequest to treat that flag/sentinel as
retryable so the retry branch can execute.
| let isTimeoutFetchError = false; | ||
| let res: Response | undefined; | ||
| let duration = 0; | ||
| let json: any; |
Contributor
There was a problem hiding this comment.
let json: any violates the no-any guideline — prefer unknown
res.json() returns any, but the outer binding can be narrowed safely:
🔧 Proposed fix
- let json: any;
+ let json: unknown;As per coding guidelines: "Never use any or as any type assertions in TypeScript code unless absolutely necessary."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let json: any; | |
| let json: unknown; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/src/chat/chat.ts` at line 4849, The binding "let json: any"
should use a safer type: change it to "let json: unknown" and then narrow it
after calling res.json() before using it (e.g., with a type guard, instanceof
checks, or explicit validation) so downstream code operates on a known shape;
update the code around the res.json() call (where "json" is assigned) to perform
the necessary runtime checks/casts and only then access properties or cast to a
specific interface.
steebchen
force-pushed
the
feat-investigate-timeout-retry-tracking
branch
from
February 23, 2026 06:50
4d8f520 to
f846078
Compare
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Line 4824: The local variable declaration "let json: any" should be changed to
"let json: unknown" and all subsequent uses of json must be properly narrowed
(e.g., via type guards, instanceof checks, or casting after validation) before
accessing properties or passing to functions; update the code around the
res.json() call so the result is assigned to the variable named json (now typed
unknown) and add explicit narrowing where functions or code paths consume json
to satisfy TypeScript's no-any guideline.
- Around line 5637-5647: The retry decision is using the successful HTTP status
res.status (200) so isRetryableError returns false; change the object passed to
shouldRetryRequest in the success-body-timeout branch (where
willRetrySuccessBodyTimeoutNonStreaming is computed) to use statusCode: 0
instead of res.status so the call to shouldRetryRequest/ isRetryableError treats
this as a transport-level retryable failure; update references to
willRetrySuccessBodyTimeoutNonStreaming and any related logging/flags (retried,
retriedByLogId) accordingly to reflect the retry path when body-read timeouts
occur (look for shouldRetryRequest, res.status,
willRetrySuccessBodyTimeoutNonStreaming).
- Around line 5791-5813: The retry exit path can leave json undefined even if
res.ok is true, so add a guard immediately after the existing if (!res ||
!res.ok) block to check for json (and usedProvider/url) and return a 502 JSON
error (e.g., code "no_response_body" or similar) instead of falling through;
update the comment that currently states "json variable is already set from
inside the retry loop" to note that json may be undefined if all attempts timed
out, and reference the symbols res, json, usedProvider, url and the downstream
parseProviderResponse call to ensure the check is performed before
parseProviderResponse(...) is called.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/gateway/src/chat/tools/retry-with-fallback.spec.ts (1)
212-230: Optional: add explicitgetErrorTypecoverage for 401/403.
getErrorType(401)andgetErrorType(403)fall through to"upstream_error"via the default branch, and this is implicitly covered by the400/404cases in the existing catch-all test. However, since 401/403 now have distinct retry semantics elsewhere, explicit assertions would make the intent clearer and guard against accidental future branching.✅ Suggested test addition
it("returns upstream_error for other status codes", () => { expect(getErrorType(400)).toBe("upstream_error"); expect(getErrorType(404)).toBe("upstream_error"); + expect(getErrorType(401)).toBe("upstream_error"); + expect(getErrorType(403)).toBe("upstream_error"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/retry-with-fallback.spec.ts` around lines 212 - 230, Add explicit assertions for 401 and 403 in the getErrorType tests: update the describe("getErrorType") block (where getErrorType is exercised) to include expectations that getErrorType(401) and getErrorType(403) each return "upstream_error"—either by adding two new it cases or expanding the existing catch-all test—so these status codes are explicitly covered and protected from future branching changes.apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts (1)
84-86: Test phrase relies on the broad"billing"match.
"Please check your billing settings"passes only becauseerrorText.includes("billing")fires. If the"billing"keyword is made more specific (as suggested in the implementation), this test case should be updated to use a phrase that matches the narrowed pattern, or a real-world provider message should be used here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts` around lines 84 - 86, The test relies on a broad "billing" substring match; update the test input used with getFinishReasonFromError(400, ...) to a string that matches the tightened billing detection (for example use a realistic provider message such as "Payment method declined: please update your billing information" or "Billing issue: payment method declined") so the billing branch still triggers and the expectation toBe("upstream_error") remains valid; adjust the test string in get-finish-reason-from-error.spec.ts where getFinishReasonFromError is called.apps/gateway/src/chat/tools/get-finish-reason-from-error.ts (1)
65-73:"billing"substring is too broad — likely to produce false positives.
"credit balance"and"insufficient_quota"are sufficiently specific, but"billing"matches any message that contains that word (e.g.,"billing address invalid","billing account not configured","check billing details for your organization"). These could be genuine gateway-configuration or client errors that should not be retried on other providers.Consider tightening the match to phrases that reliably indicate a provider-side credit/quota exhaustion, or explicitly enumerate the known provider patterns:
♻️ Suggested refinement
if ( errorText.includes("credit balance") || - errorText.includes("insufficient_quota") || - errorText.includes("billing") + errorText.includes("insufficient_quota") || + errorText.includes("billing hard limit") || + errorText.includes("billing_hard_limit_reached") || + errorText.includes("check your billing") ) { return "upstream_error"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.ts` around lines 65 - 73, The check in get-finish-reason-from-error.ts is too broad because matching the substring "billing" can produce false positives; update the condition that inspects errorText (the block that currently returns "upstream_error") to remove the generic "billing" check and instead match a tightened set of provider-credit phrases or explicit provider patterns (e.g., provider-specific messages indicating exhausted credits/insufficient funds or phrased like "billing balance", "billing account suspended", "billing quota", or known provider error keys). Modify the logic around errorText.includes(...) to use an explicit whitelist of phrases/regexes for credit/quota exhaustion and include those exact identifiers (keep "credit balance" and "insufficient_quota"), and ensure the branch still returns "upstream_error" only for those tightened matches.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/chat/tools/retry-with-fallback.spec.tsapps/gateway/src/chat/tools/retry-with-fallback.ts
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts`:
- Around line 84-86: The test relies on a broad "billing" substring match;
update the test input used with getFinishReasonFromError(400, ...) to a string
that matches the tightened billing detection (for example use a realistic
provider message such as "Payment method declined: please update your billing
information" or "Billing issue: payment method declined") so the billing branch
still triggers and the expectation toBe("upstream_error") remains valid; adjust
the test string in get-finish-reason-from-error.spec.ts where
getFinishReasonFromError is called.
In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.ts`:
- Around line 65-73: The check in get-finish-reason-from-error.ts is too broad
because matching the substring "billing" can produce false positives; update the
condition that inspects errorText (the block that currently returns
"upstream_error") to remove the generic "billing" check and instead match a
tightened set of provider-credit phrases or explicit provider patterns (e.g.,
provider-specific messages indicating exhausted credits/insufficient funds or
phrased like "billing balance", "billing account suspended", "billing quota", or
known provider error keys). Modify the logic around errorText.includes(...) to
use an explicit whitelist of phrases/regexes for credit/quota exhaustion and
include those exact identifiers (keep "credit balance" and
"insufficient_quota"), and ensure the branch still returns "upstream_error" only
for those tightened matches.
In `@apps/gateway/src/chat/tools/retry-with-fallback.spec.ts`:
- Around line 212-230: Add explicit assertions for 401 and 403 in the
getErrorType tests: update the describe("getErrorType") block (where
getErrorType is exercised) to include expectations that getErrorType(401) and
getErrorType(403) each return "upstream_error"—either by adding two new it cases
or expanding the existing catch-all test—so these status codes are explicitly
covered and protected from future branching changes.
steebchen
force-pushed
the
feat-investigate-timeout-retry-tracking
branch
from
February 23, 2026 12:19
9bf2658 to
4686802
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/gateway/src/chat/chat.ts (3)
5645-5655:⚠️ Potential issue | 🔴 CriticalPass
statusCode: 0for body-read timeouts —res.status(200) prevents retries entirely.After the guard at line 5624,
res.okis guaranteedtrue, sores.statusis 200 (or another 2xx).isRetryableError(200)returnsfalse, makingshouldRetryRequestalways returnfalse. This means the success-response body timeout can never be retried, defeating the purpose of moving the parse into the retry loop.Use
statusCode: 0(the same convention used for fetch-level timeouts at line 5008) to signal a transport-level failure:Proposed fix
const willRetrySuccessBodyTimeoutNonStreaming = shouldRetryRequest({ requestedProvider, noFallback, - statusCode: res.status, + statusCode: 0, retryCount: retryAttempt, remainingProviders: (routingMetadata?.providerScores.length ?? 0) - failedProviderIds.size - 1, usedProvider, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 5645 - 5655, The retry decision for body-read timeouts incorrectly uses res.status (a 2xx) in the shouldRetryRequest call inside the construction of willRetrySuccessBodyTimeoutNonStreaming, causing isRetryableError to return false; change that call to pass statusCode: 0 when the timeout path is hit (same convention used for fetch-level timeouts) so shouldRetryRequest/ isRetryableError treat it as a transport-level failure and allow retries for the parse/body-read timeout path (update the call around willRetrySuccessBodyTimeoutNonStreaming to use statusCode: 0 instead of res.status when the body-read timeout branch is executed).
5799-5819:⚠️ Potential issue | 🟠 MajorMissing guard for
json === undefinedafter retry exhaustion with success-body timeouts.Once the
statusCode: 0fix is applied toshouldRetryRequest, retries become possible for success-body timeouts. If every fallback provider's body parse also times out, the loop breaks withjsonstillundefinedwhileresis defined andres.ok === true. The existing guard at line 5799 only covers!res || !res.okand won't protect against this case, soparseProviderResponse(..., json, ...)at line 5830 would crash withundefined.Add a guard after the
!res || !res.okblock:Proposed fix
if (!res || !res.ok) { // All retries exhausted return c.json( { error: { message: "All provider attempts failed", type: "upstream_error", param: null, code: "all_providers_failed", }, }, 502, ); } + + if (json === undefined) { + return c.json( + { + error: { + message: "All provider attempts failed (response body timeout)", + type: "upstream_timeout", + param: null, + code: "all_providers_failed", + }, + }, + 504, + ); + } // After successful retry loop, all provider variables are guaranteed set🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` around lines 5799 - 5819, After the retry loop, add a guard that checks whether the parsed response payload (json) is undefined even when res is present and res.ok is true — this can happen when shouldRetryRequest allows retries for success-body timeouts and all provider body parses timed out; in the block following the existing if (!res || !res.ok) check, detect json === undefined and return an upstream error JSON response (similar shape to the existing "All provider attempts failed" error) or throw a clear error before calling parseProviderResponse so parseProviderResponse(...) and subsequent code never receive undefined; reference variables/resolvers: json, res, usedProvider, url, parseProviderResponse, and the retry loop/shouldRetryRequest logic to locate the fix.
4829-4829:let json: anyviolates the no-anyguideline — preferunknown.Proposed fix
- let json: any; + let json: unknown;As per coding guidelines: "Never use
anyoras anytype assertions in TypeScript code unless absolutely necessary."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/chat.ts` at line 4829, The declaration "let json: any" should be changed to "let json: unknown" and all subsequent uses must be narrowed or validated before treating it as a specific type; locate the variable named "json" in this file (apps/gateway/src/chat/chat.ts) and replace its type with unknown, then add appropriate type guards, JSON.parse error handling, or schema validation (e.g., runtime checks or zod/io-ts) where "json" is read or properties accessed, and only then cast to the concrete interface or type.
🧹 Nitpick comments (1)
apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts (1)
72-87: Add a negative test to document the case-sensitive boundary of the"billing"keyword check.The implementation correctly uses case-sensitive substring matching for billing-related keywords. While the existing negative test at line 89–92 covers general 400 errors, there's no explicit test documenting that uppercase variants like
"Billing"fall through togateway_error. Adding a test case to document this case-sensitive boundary would prevent accidental regressions if the check is changed.🧪 Suggested additional test case
+ it("does not misclassify unrelated 400 errors that incidentally contain 'billing' substring", () => { + // Case-sensitivity: uppercase "Billing" should NOT match the lowercase check + expect( + getFinishReasonFromError(400, "Billing validation failed"), + ).toBe("gateway_error"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts` around lines 72 - 87, Add a negative unit test in the existing spec for getFinishReasonFromError that asserts case-sensitive behavior: verify that a 400 error whose message contains "Billing" (capital B) and/or "BILLING" returns "gateway_error" (not "upstream_error"), so the current case-sensitive substring check remains documented and guarded against accidental changes to getFinishReasonFromError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/gateway/src/chat/chat.ts`:
- Around line 5645-5655: The retry decision for body-read timeouts incorrectly
uses res.status (a 2xx) in the shouldRetryRequest call inside the construction
of willRetrySuccessBodyTimeoutNonStreaming, causing isRetryableError to return
false; change that call to pass statusCode: 0 when the timeout path is hit (same
convention used for fetch-level timeouts) so shouldRetryRequest/
isRetryableError treat it as a transport-level failure and allow retries for the
parse/body-read timeout path (update the call around
willRetrySuccessBodyTimeoutNonStreaming to use statusCode: 0 instead of
res.status when the body-read timeout branch is executed).
- Around line 5799-5819: After the retry loop, add a guard that checks whether
the parsed response payload (json) is undefined even when res is present and
res.ok is true — this can happen when shouldRetryRequest allows retries for
success-body timeouts and all provider body parses timed out; in the block
following the existing if (!res || !res.ok) check, detect json === undefined and
return an upstream error JSON response (similar shape to the existing "All
provider attempts failed" error) or throw a clear error before calling
parseProviderResponse so parseProviderResponse(...) and subsequent code never
receive undefined; reference variables/resolvers: json, res, usedProvider, url,
parseProviderResponse, and the retry loop/shouldRetryRequest logic to locate the
fix.
- Line 4829: The declaration "let json: any" should be changed to "let json:
unknown" and all subsequent uses must be narrowed or validated before treating
it as a specific type; locate the variable named "json" in this file
(apps/gateway/src/chat/chat.ts) and replace its type with unknown, then add
appropriate type guards, JSON.parse error handling, or schema validation (e.g.,
runtime checks or zod/io-ts) where "json" is read or properties accessed, and
only then cast to the concrete interface or type.
---
Nitpick comments:
In `@apps/gateway/src/chat/tools/get-finish-reason-from-error.spec.ts`:
- Around line 72-87: Add a negative unit test in the existing spec for
getFinishReasonFromError that asserts case-sensitive behavior: verify that a 400
error whose message contains "Billing" (capital B) and/or "BILLING" returns
"gateway_error" (not "upstream_error"), so the current case-sensitive substring
check remains documented and guarded against accidental changes to
getFinishReasonFromError.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/gateway/src/chat/chat.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.spec.tsapps/gateway/src/chat/tools/get-finish-reason-from-error.tsapps/gateway/src/chat/tools/retry-with-fallback.spec.tsapps/gateway/src/chat/tools/retry-with-fallback.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/gateway/src/chat/tools/get-finish-reason-from-error.ts
- apps/gateway/src/chat/tools/retry-with-fallback.ts
- apps/gateway/src/chat/tools/retry-with-fallback.spec.ts
…arios Fix critical gaps in timeout handling for non-streaming requests where timeouts during response body reading were either not retried at all or not properly tracked in logs. Issues Fixed: 1. Error response body timeout - now properly retries with fallback 2. Success response body timeout - moved inside retry loop to enable retry Changes: - Moved success response body parsing inside the retry loop - Added retry decision logic (shouldRetryRequest) for both timeout cases - Added proper log tracking (retried, retriedByLogId) for both cases - Added routing attempt tracking for observability - Changed from immediate return to continue for retry iteration All timeout scenarios now create logs with proper retry metadata, link retried attempts via finalLogId, and attempt retries with fallback providers. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…level failure The success response body timeout path was using res.status (200) for error details, routing attempts, and key health reporting. This is semantically incorrect since the upstream request succeeded — the failure is a transport-level timeout reading the body. Changed to status code 0 (consistent with fetch-level timeouts) which maps to "network_error" via getErrorType(). Also removed the reportKeyError call since the provider key is not at fault when a body read times out on a successful response. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Provider-specific auth and billing errors (401, 403, and 400 with billing-related messages like "credit balance too low") should be retried on other providers rather than returned to the caller. Previously, a 400 from Anthropic with "credit balance is too low" was classified as "gateway_error" and returned as a 500 to the caller without any retry attempt. Changes: - isRetryableError: add 401 and 403 as retryable status codes - getFinishReasonFromError: classify 401/403 as "upstream_error" and detect billing-related 400 errors (credit balance, insufficient_quota, billing) as "upstream_error" instead of "gateway_error" - Update tests for new retry and classification behavior Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
steebchen
force-pushed
the
feat-investigate-timeout-retry-tracking
branch
from
February 24, 2026 06:39
4686802 to
01087e9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
retried,retriedByLogId) and attempt fallback via existing routing logicDetails
Two gaps existed in non-streaming timeout handling:
1. Error response body timeout (
res.text()times out on non-ok response)The code returned 504 immediately without calling
shouldRetryRequest()or settingretried/retriedByLogIdon the log. Now it evaluates retry conditions, tracks properly, and continues to the next provider if eligible.2. Success response body timeout (
res.json()times out on ok response)Body parsing happened after the retry loop exited, so no retry was possible. Moved it inside the loop so timeouts here are treated like any other retryable error.
Test plan
retried: trueandretriedByLogIdpointing to the final log ID when retried🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests